Skip to content

feat: implement blob/retrieve capability#284

Merged
alanshaw merged 11 commits intomainfrom
ash/feat/implement-blob-retrieve
Oct 16, 2025
Merged

feat: implement blob/retrieve capability#284
alanshaw merged 11 commits intomainfrom
ash/feat/implement-blob-retrieve

Conversation

@alanshaw
Copy link
Member

This PR add a blob/retrieve capability handler for service retrievals (see storacha/RFC#68). It is very similar to the space/content/retrieve handler except it does accept byte range requests, only allows invocations where the resource is the service itself and is not associated with any egress billing.

@alanshaw alanshaw requested a review from a team October 15, 2025 20:27
@alanshaw alanshaw requested a review from frrist as a code owner October 15, 2025 20:27
Copy link
Member

@frrist frrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comments in pkg/service/retrieval/ucan/blob_retrieve.go, they are non-blocking, but I'd like a bit of discussion before merge.


I have a hunch this test failure:

=== RUN   TestFXBlobRetrieve/blob/retrieve
    ucan_fx_test.go:196: 
        	Error Trace:	/Users/runner/work/piri/piri/pkg/service/retrieval/ucan_fx_test.go:196
        	Error:      	Received unexpected error:
        	            	sending message: doing HTTP request: Get "http://localhost:8080/piece/bafkreigrspvthgxgp4uxx3v4y7oo3cu3cxucoolsgx56ik6atues2e76ae": dial tcp [::1]:8080: connect: connection refused
        	Test:       	TestFXBlobRetrieve/blob/retrieve
=== NAME  TestFXBlobRetrieve

Is caused by many of our testing "things" using 8080 as a default, and as each one spins up/down we get some thrash thus these errors. In the future, we can remove this flake by letting the OS pick a port.

Comment on lines +29 to +33
getOptions := []blobstore.GetOption{}
if cfg.ByteRange.Start > 0 || cfg.ByteRange.End != nil {
getOptions = append(getOptions, blobstore.WithRange(cfg.ByteRange.Start, cfg.ByteRange.End))
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to guess this validation was added since we don't pass a range here: https://github.com/storacha/piri/pull/284/files#r2433988099 ? Seems good to have regardless.

Copy link
Member Author

@alanshaw alanshaw Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it doesn't matter actually - I added this when I had a test failure and thought this was requesting a range of 0 bytes. However you actually end up with a range 0- which is effectively the same as not specifying a range.

@frrist
Copy link
Member

frrist commented Oct 15, 2025

Per my previous comment about test flake, I filed #285. can rebase this change on there to clear up

@alanshaw alanshaw force-pushed the ash/feat/implement-access-grant branch from bad4c57 to afb0092 Compare October 16, 2025 16:21
@alanshaw alanshaw force-pushed the ash/feat/implement-blob-retrieve branch from 6cca3d9 to 9ec0bce Compare October 16, 2025 16:26
Base automatically changed from ash/feat/implement-access-grant to main October 16, 2025 22:30
@alanshaw alanshaw merged commit 2b45a15 into main Oct 16, 2025
8 checks passed
@alanshaw alanshaw deleted the ash/feat/implement-blob-retrieve branch October 16, 2025 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants